Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bubble up error message #112

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

dpr-0
Copy link

@dpr-0 dpr-0 commented Jan 28, 2024

Fixes #99

Re: #99 (comment)

Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions/comments:

  1. How does the output look like after this changes?
  2. Since this except seems higher up in the call stack, can it be triggered by other failures that are not related to sha validation?
  3. IIRC the goal of the sha validation is to find out if the given commit exists in the repo, so instead of saying "Error validate sha", I would say something like f"Cannot locate sha commit. Are you inside a {config['repo']} repo?" (this is assuming this error is triggered only by failed sha validation, if not it should be moved deeper in the function that checks)
  4. Can you add a test for this?

@dpr-0
Copy link
Author

dpr-0 commented Jan 28, 2024

Hi @ezio-melotti, thanks for reviewing my PR.

For your questions,

  1. The current output looks like this:

    $ cherry_picker 
    🐍 🍒 ⛏
    Error validate sha
    The sha listed in the branch name, 7f777ed95a19224294949e1b4ce56bbffcb1fe9f, is not present in the repository
  2. and 3.

    The IRC will be raised when validate_sha(self.config["check_sha"]) or self.get_state_and_verify() fail, because both may raise ValueError, so maybe saying f"Cannot locate sha commit or get state. Are you inside a {config['repo']} repo?"?

4 . Yes, the test would be like this (tested):

def test_cli_invoked_invalid_repo():
    with pytest.raises(subprocess.CalledProcessError) as exc_info:
        subprocess.check_output("cherry_picker")
    assert exc_info.value.output.decode().startswith(
        "\U0001F40D \U0001F352 \u26CF\nCannot locate sha commit or get state. Are you inside a cpython repo?"
    )

@ezio-melotti
Copy link
Member

ezio-melotti commented Jan 28, 2024

Regarding the error message: it looks like the original error message (at least for the invalid sha) is clear enough, so perhaps there is no need to add the f"Cannot locate sha commit or get state. Are you inside a {config['repo']} repo?". Is this true also for the "get state"? (BTW, it's not clear just by reading the proposed message what "get state" is supposed to do, why it failed, and how to fix the problem.)

Regarding the test, there is actually a test already, but only checks the exception type:

def test_is_not_cpython_repo():
# use default CPython sha to fail on this repo
with pytest.raises(InvalidRepoException):
CherryPicker("origin", "22a594a0047d7706537ff2ac676cdc0f1dcb329c", ["3.6"])

You can use pytest.raises to check that the error message mentions the invalid sha and (possibly separately) whatever error message is raised when getting the state fails.

@dpr-0
Copy link
Author

dpr-0 commented Jan 29, 2024

Is this true also for the "get state"?

I think the original error message is clear too:

raise ValueError(
f"Run state cherry-picker.state={state.name} in Git config "
"is not known.\nPerhaps it has been set by a newer "
"version of cherry-picker. Try upgrading.\n"
"Valid states are: "
f'{", ".join(s.name for s in self.ALLOWED_STATES)}. '
"If this looks suspicious, raise an issue at "
"https://github.com/python/cherry-picker/issues/new.\n"
"As the last resort you can reset the runtime state "
"stored in Git config using the following command: "
"`git config --local --remove-section cherry-picker`"
)

If we both agree there is no need to add the summary of error message, I can just enhance existing tests.

@dpr-0
Copy link
Author

dpr-0 commented Mar 22, 2024

@ezio-melotti

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"You're not inside a cpython repo right now! 🙅" says cherry_picker when I'm in my cpython git repo
2 participants